-
Notifications
You must be signed in to change notification settings - Fork 11
Feature/cleanup and recursion #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
4a2adc2 to
8333e21
Compare
|
Hi there 👋🏼 Thank you for working on this. I ran a few tests using this branch, and unfortunately the problem persists. Going to try and share as much information as possible, progressively, as time allows me to investigate further. Current knowledge: we see infinite recursion in Edit: It looks like the smallest reproducible test case might be any message that references google.protobuf.Struct. This message type is indirectly self-referential. Edit 2: It's possible that the solution is related to adding a conditional based on |> Enum.reduce(state, fn %{mod: mod, symbol: symbol}, state ->
symbol = Util.trim_symbol(symbol)
+ if State.has_symbol?(state, symbol) do
+ state
+ else
response =
if symbol in nested_types do
build_response(parent_symbol, module)
else
build_response(symbol, mod)
end
state
|> Extensions.add_extensions(symbol, mod)
|> State.add_symbols(%{symbol => response})
|> State.add_files(%{(symbol <> ".proto") => response})
|> trace_message_refs(symbol, mod)
+ end
end)Unfortunately, while this addition avoids infinite recursion in my test cases, we get a different error (that may be completely unrelated, because it happens in this branch and on the latest release). Edit 3: I see now that #59 is a better version of this suggestion. |
* Don't build symbols we already have in the state * update test for file merging in cycles
|
@aj-foster yeah, I had some work in that direction in #59 already, which I'm embarrassed I didn't think to combine these before your comment. They are both in this PR now. It does make sense that we would need both solutions as the recursion occurs in two places. Thanks for helping keep an eye on this, it is really appreciated. |
This is an attempt to resolve the circular recursion case, in this PR:
When files participate in a cycle, causing the reflection tree to instead be a graph, we combine those participating file descriptors into a single descriptor.